Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds bsd support #1

Merged
merged 1 commit into from Jan 10, 2017
Merged

Adds bsd support #1

merged 1 commit into from Jan 10, 2017

Conversation

conradkleinespel
Copy link
Contributor

@conradkleinespel conradkleinespel commented Jan 7, 2017

Thanks for this handy library !

This commit adds BSD support (FreeBSD, NetBSD, DragonFly, and possibly others).

Let me know if you have any questions.

Copy link
Owner

@wesleywiser wesleywiser left a comment

Generally this looks good 👍 Thanks for the PR!

pub fn get_executable_path() -> Option<PathBuf> {
// FreeBSD
let mut mib = [0; 4];
mib[0] = CTL_KERN;
Copy link
Owner

@wesleywiser wesleywiser Jan 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be written as let mut mib = [ CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1 ];?

let mut path = String::new();
let len = unsafe { strlen(buf.as_ptr()) };
let mut i = 0;
while i < len {
Copy link
Owner

@wesleywiser wesleywiser Jan 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If buf is declared as a Vec instead of a raw array, you can just use buf.set_len(len) here instead. The macos module uses this approach.

pb.push(path);
Some(pb)
} else {
// NetBSD
Copy link
Owner

@wesleywiser wesleywiser Jan 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This says NetBSD but the module declaration in lib.rs says OpenBSD. Is that correct?

@conradkleinespel
Copy link
Contributor Author

@conradkleinespel conradkleinespel commented Jan 8, 2017

I didn't know all of that was possible. Thanks for your help 👍

Regarding OpenBSD, that was a mistake on my part. From what I can read on the internet, there seems to be no way to get the full executable path on OpenBSD, so I've removed references to it.

I've also added some more information about FreeBSD, which works with sysctl OR /proc/curproc/file depending on the system configuration.

@conradkleinespel
Copy link
Contributor Author

@conradkleinespel conradkleinespel commented Jan 8, 2017

Woops, forgot to use set_len. Will commit again in a few minutes.

@conradkleinespel
Copy link
Contributor Author

@conradkleinespel conradkleinespel commented Jan 8, 2017

And I haven't tested this properly on NetBSD and DragonflyBSD yet. Maybe I should do that before you choose to merge, no ? I'm in the process of setting up NetBSD / Dragonfly VPS so I can test this.

let mut buf: Vec<u8> = Vec::with_capacity(1024);
let mut cb = 1024 as size_t;
let result = unsafe {
sysctl(mib.as_ptr(),
Copy link
Owner

@wesleywiser wesleywiser Jan 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern at this point, is that on NetBSD & DragonflyBSD, we're calling this sysctl that only works on FreeBSD. It seems that could have unintended consequences. I suggest we use the cfg!() macro to do platform detection. For example:

if cfg!(target_os = "freebsd") {
//Freebsd sysctl call
} else {
//fallback to readlink on /proc/curproc/{exe,file}
}

Copy link
Contributor Author

@conradkleinespel conradkleinespel Jan 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll do that once I have my different BSD environments running. FreeBSD will probably need an entirely different function since it uses either sysctl or a /proc file. Either way, I'll update the PR once I've pushed these modifications. Thanks for your feedback !

@wesleywiser
Copy link
Owner

@wesleywiser wesleywiser commented Jan 8, 2017

Yes, if you could test it, that would be great! I'm not really familiar with the BSDs and I'm unsure how to get a Rust environment running on one.

@conradkleinespel
Copy link
Contributor Author

@conradkleinespel conradkleinespel commented Jan 8, 2017

@wesleywiser Will do ! I'm looking at using Vultr.com, which supports custom ISOs (so any OS you want, basically) and I think that rustup works fine on BSD, so installing should hopefully be easy enough.

@wesleywiser
Copy link
Owner

@wesleywiser wesleywiser commented Jan 8, 2017

@conradkleinespel
Copy link
Contributor Author

@conradkleinespel conradkleinespel commented Jan 9, 2017

@wesleywiser I've cleaned up the code separation for the different OSes. Unfortunately though, I wasn't able to build Rust on Dragonfly, and Dragonfly has no pre-built binaries available via Rustup. Also, I was not able to install NetBSD. I just don't get how the install process works.

Given how simple the code for Dragonfly and NetBSD is, I suppose it would work. The code for FreeBSD I've tested and it works for the sysctl case. I suppose that the other code path works.

If you'd prefer me to remove the code for Dragonfly, NetBSD and FreeBSD (with procfs), I'm glad to do that. I just think that if someone with more Dragonfly/NetBSD/FreeBSD (with procfs) experience comes along, they'll hopefully help us fix whatever is broken.

@wesleywiser
Copy link
Owner

@wesleywiser wesleywiser commented Jan 10, 2017

The Dragonfly & NetBSD implementations are very simple so I'm not too concerned about testing them. I'll test the FreeBSD path on my VM and then merge it.

The code looks great 👍

@wesleywiser wesleywiser merged commit 792a278 into wesleywiser:master Jan 10, 2017
@wesleywiser
Copy link
Owner

@wesleywiser wesleywiser commented Jan 10, 2017

Thanks so much!

@conradkleinespel conradkleinespel deleted the bsd branch Jan 10, 2017
@conradkleinespel
Copy link
Contributor Author

@conradkleinespel conradkleinespel commented Jan 10, 2017

Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants